Integrate sherlog memory review roadmap work - #82
Conversation
… into agent/sherlog-memory-review-stack
…nto agent/sherlog-memory-review-stack
… into agent/sherlog-memory-review-stack
…ures' into agent/sherlog-memory-review-stack
真实数据 agent-as-judge 评测结果在 4566 sessions / 247K messages 的真实库上跑了 20 个真实 query 的 A/B 对照(main vs 本 PR),又派了 6 个 agent 做真实记忆检索任务(每个 agent 自主决定查询词、自主调 sherlog、自主读 evidenceRead),然后对整体交互打分。 1. DB 兼容性:✅ 安全PR 没改 2. query-profile scoring (#71):🔴 真实数据上无可观测效果A/B 对照:20 个真实 query 中 top-1 改变 0 个,top-3 有变化的 3 个(只在 #2/#3 位),17 个 top-3 完全一致。手调权重(8/18/10、cwd 18/12、hitCount 1.75/1.25)在真实数据上几乎没影响。 agent 交互验证:6 个 agent 全程没有一个感知到 broad vs exact 的区分帮到了自己。所有 agent 报告 score 贴在 0.016 左右、彼此几乎不可区分(Agent 1:"score 全在 0.015~0.016 量级,结果之间区分度很小";Agent 3:"所有结果 score 几乎一样,难以区分")。query-profile 只在 row-level bonus 上分权,不改变 BM25 基线的低区分度。 issue #71 要求"make query profile scoring real or remove the stale abstraction"。现在 scoring 确实 real 了(代码真的 branch on 3. evidenceRead (#72):🟢 真实有效,但 session-only hit 有缺陷A/B 对照 + agent 交互:6/6 agent 都自然 follow 了 🔴 但 session-only hit 的 evidenceRead 指向 offset=0,对长 session 无用:
4. acceptance gate (#70):🟡 循环验证,不能检测真实问题合成 fixture 和 golden 是同一个人写的—— agent 交互暴露的真实问题 acceptance gate 一个都没覆盖:
5. source contracts (#73) + perf-bench (#74):🟢 稳format-drift / 隐私测试(pi/codex/claude-code 三套)写得扎实,真在测畸形 JSON、unsupported record、toolCall 泄漏。perf-bench 加 read-range/read-page 延迟探针,无性能回归(PR p95=561ms,main p95=716ms)。 agent 交互打分
6/6 agent 最终都完成了任务,平均 9.3/12。evidenceRead + read-range/read-page 是这次 PR 唯一带来真实价值的部分。 merge 建议可以 merge。evidenceRead + source contracts + perf telemetry 值这个 PR。但:
trace: |
Session-only hits (matchSeq===null, matchSource=session) previously returned read-page offset=0, which for long sessions read the session beginning (often irrelevant greetings) instead of the relevant evidence. 6/6 dogfood agents confirmed this defect on real data. When the query is available, buildEvidenceReadAction now returns read-range --query <query> instead of read-page offset=0. This lets resolveAnchorSeq use searchTopHitInSession to locate the real evidence anchor inside the session transcript. When the query finds no message hits (e.g. session-level match from compact/summary with different wording), resolveAnchorSeq falls back to seq=0 instead of throwing. Changes: - evidence-read.ts: add query to EvidenceReadAction, return read-range --query for session-only hits when query is provided - query/read.ts: resolveAnchorSeq falls back to seq=0 when query finds no hits, instead of throwing - cli.ts: pass query to buildEvidenceReadAction - acceptance-gate.ts: session-only-compact-context fixture expects read-range; readContextIfNeeded supports read-range with query for session-only hits - dogfood-eval-core.ts: desiredContextMode returns read-range for session-only hits in auto mode - dogfood-schema.ts: add query field to DogfoodExpectedContext Fixes #83
Additional fix: #83 session-only evidenceRead (pushed to this branch)Commit ProblemSession-only hits ( Fix
Verified
Known limitationWhen session-level hits come from compact/summary text, query terms may not appear verbatim in raw messages (e.g. compact says "ranking weights" but messages only say "ranking"). FTS returns 0 hits → fall back to |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/query/read.ts">
<violation number="1" location="src/query/read.ts:101">
P2: Falling back to `return 0` makes `anchorSeq` ambiguous — identical to a genuine match at seq=0. The `>>` marker and JSON payload will incorrectly suggest the query landed at seq=0 when no message matched.</violation>
</file>
<file name="eval/dogfood-schema.ts">
<violation number="1" location="eval/dogfood-schema.ts:176">
P3: `context.query` validation does not trim whitespace. Whitespace-only query strings are accepted as valid.</violation>
<violation number="2" location="eval/dogfood-schema.ts:176">
P2: `context.query` is parsed but not counted as an expected assertion. Entries using only `expected.context.query` are incorrectly rejected.</violation>
</file>
<file name="eval/perf-bench.ts">
<violation number="1" location="eval/perf-bench.ts:258">
P2: p95 calculation is hardcoded to max for all sample sizes. This turns percentile telemetry into worst-case only and can mask real latency distribution changes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Query found no message-level hit (e.g. session-level match from title or | ||
| // compact). Fall back to seq=0 so read-range still returns a usable window | ||
| // instead of throwing — the caller can page forward if needed. | ||
| return 0; |
There was a problem hiding this comment.
P2: Falling back to return 0 makes anchorSeq ambiguous — identical to a genuine match at seq=0. The >> marker and JSON payload will incorrectly suggest the query landed at seq=0 when no message matched.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/query/read.ts, line 101:
<comment>Falling back to `return 0` makes `anchorSeq` ambiguous — identical to a genuine match at seq=0. The `>>` marker and JSON payload will incorrectly suggest the query landed at seq=0 when no message matched.</comment>
<file context>
@@ -95,6 +95,10 @@ function resolveAnchorSeq(
+ // Query found no message-level hit (e.g. session-level match from title or
+ // compact). Fall back to seq=0 so read-range still returns a usable window
+ // instead of throwing — the caller can page forward if needed.
+ return 0;
}
</file context>
| if (limit) context.limit = limit; | ||
| const mustContain = readStringArray(value.mustContain); | ||
| if (mustContain) context.mustContain = mustContain; | ||
| if (typeof value.query === "string" && value.query.length > 0) context.query = value.query; |
There was a problem hiding this comment.
P2: context.query is parsed but not counted as an expected assertion. Entries using only expected.context.query are incorrectly rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At eval/dogfood-schema.ts, line 176:
<comment>`context.query` is parsed but not counted as an expected assertion. Entries using only `expected.context.query` are incorrectly rejected.</comment>
<file context>
@@ -153,6 +173,7 @@ function parseContext(value: unknown): DogfoodExpectedContext | undefined {
if (limit) context.limit = limit;
const mustContain = readStringArray(value.mustContain);
if (mustContain) context.mustContain = mustContain;
+ if (typeof value.query === "string" && value.query.length > 0) context.query = value.query;
return Object.keys(context).length > 0 ? context : undefined;
</file context>
| // 小样本下 p95 数学意义薄弱: 直接取 max 作为 worst-case 近似 | ||
| if (samplesMs.length === 0) return 0; | ||
| if (p >= 0.99) return Math.max(...samplesMs); | ||
| if (p >= 0.95) return Math.max(...samplesMs); |
There was a problem hiding this comment.
P2: p95 calculation is hardcoded to max for all sample sizes. This turns percentile telemetry into worst-case only and can mask real latency distribution changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At eval/perf-bench.ts, line 258:
<comment>p95 calculation is hardcoded to max for all sample sizes. This turns percentile telemetry into worst-case only and can mask real latency distribution changes.</comment>
<file context>
@@ -125,9 +253,9 @@ function median(sorted: number[]): number {
+ // 小样本下 p95 数学意义薄弱: 直接取 max 作为 worst-case 近似
if (samplesMs.length === 0) return 0;
- if (p >= 0.99) return Math.max(...samplesMs);
+ if (p >= 0.95) return Math.max(...samplesMs);
const sorted = [...samplesMs].sort((a, b) => a - b);
const idx = Math.min(sorted.length - 1, Math.floor(p * sorted.length));
</file context>
| if (limit) context.limit = limit; | ||
| const mustContain = readStringArray(value.mustContain); | ||
| if (mustContain) context.mustContain = mustContain; | ||
| if (typeof value.query === "string" && value.query.length > 0) context.query = value.query; |
There was a problem hiding this comment.
P3: context.query validation does not trim whitespace. Whitespace-only query strings are accepted as valid.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At eval/dogfood-schema.ts, line 176:
<comment>`context.query` validation does not trim whitespace. Whitespace-only query strings are accepted as valid.</comment>
<file context>
@@ -153,6 +173,7 @@ function parseContext(value: unknown): DogfoodExpectedContext | undefined {
if (limit) context.limit = limit;
const mustContain = readStringArray(value.mustContain);
if (mustContain) context.mustContain = mustContain;
+ if (typeof value.query === "string" && value.query.length > 0) context.query = value.query;
return Object.keys(context).length > 0 ? context : undefined;
</file context>
| if (typeof value.query === "string" && value.query.length > 0) context.query = value.query; | |
| const query = readNonEmptyString(value, "query"); | |
| if (query) context.query = query; |
1 participant
Purpose
This is a draft integration PR for unified review only. It is not a request to merge immediately.
The goal is to make the completed and verified Sherlog memory-review roadmap work visible in one branch while keeping the individual PRs and issue tracker intact for whichever merge strategy the reviewer chooses.
Included roadmap issues
Covered issues from the deep-review roadmap:
No issue is being closed by this PR. The issues should remain open until the chosen merge path lands and the tracker is reconciled.
Included / related PRs
f40fa32). Still open againstmain.findJSON (b765fc5, integration merge322dc6b). Still open againstmain.592c700, integration merge1873fa7). Still open againstmain.dc7fbc1, integration merge1c3abf7). Still open againstmain.80a970e). This PR is now merged intoagent/sherlog-memory-review-stackbecause its base was the integration branch, notmain.1c97c73). Still open on the stacked branch path.bae64c6, integration merge9657abc). Still open againstmain.Integration branch
mainagent/sherlog-memory-review-stack9657abc44d19103abe47b5925f1e50e0a75bd334Verification
Ran after merging the verified branches into
agent/sherlog-memory-review-stack:npm run check-> passed: TypeScript plus 32 test files / 212 tests.npm run eval:acceptance-> passed: 6/6 acceptance cases, 0 failures.git diff --check origin/agent/sherlog-memory-review-stack...HEAD-> passed.The individual PRs also had passing GitHub Actions and AI review checks when checked before this integration PR was opened.
Suggested merge path
Option A: unified review / integration path
main.Option B: individual review path
main: Add evidence-level acceptance eval gate #75, Add deterministic evidence reads to find JSON #76, Extend perf bench operation cost telemetry #77, Document source adapter projection contracts #78, Add source adapter format-drift smoke tests #81.Risks / review focus
eval/acceptance-gate.ts,eval/acceptance-gate.test.ts, andeval/run-acceptance-eval.tsto ensure the gate is strict enough without being brittle.src/ranking.ts,src/ranking.test.ts, anddocs/RANKING_WEIGHTS.mdfor query-profile weighting and tie-break behavior.src/cli.ts,src/evidence-read.ts, and tests to ensurefindJSON evidence reads are additive and do not change the top-level command surface.docs/SOURCE_CONTRACTS.md,src/sources/codex.test.ts,src/sources/claude-code.test.ts, andsrc/sources/pi.test.tsfor projected text boundaries and malformed/unsupported record handling.eval/perf-bench.tsfor whether sync/find/read/freshness timing and payload measures are useful enough for future regressions.Trace:
request_id: cxs_create_integration_pr_20260626T064214Z_api_appendSummary by cubic
Integrates the Sherlog memory-review roadmap, anchors session-only evidence with
read-range --query, and removes the ineffective query-profile scoring branch while keeping phrase/coverage signals.New Features
npm run eval:acceptanceruns fixtures acrosscodex,claude-code, andpi, checkssourceId/sessionRef/matchSource/matchSeq, and verifies read-range context for message and session-only hits.find --json: each hit includes anevidenceReadaction; message hits useread-range, session-only hits useread-range --query(fallback toread-pageif no query), with source-awaresessionRef.docs/RANKING_WEIGHTS.md.npm run eval:perfreports find p50/p95, scanned message count, top hit, and read probe p95; plus coverage/freshness costs. Supports--source,--runs,--read-runs.docs/SOURCE_CONTRACTS.mdand tests ensure malformed/unsupported/private records are filtered forcodex,claude-code, andpi.Migration
find --jsonadds anevidenceReadfield; session-only hits now returnread-range --queryinstead ofread-page. Update consumers that assume exact payload shapes.Written for commit 3d9f782. Summary will update on new commits.